Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge rule and aspect validation output groups #19630

Closed

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Sep 26, 2023

By merging the special _validation output groups across a rule and its attached aspects, aspects and rules can simultaneously use validation actions.

Fixes #19624

@fmeum fmeum requested a review from a team as a code owner September 26, 2023 07:43
@fmeum fmeum requested review from katre, ahumesky and a team and removed request for a team and katre September 26, 2023 07:43
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions labels Sep 26, 2023
By merging the special `_validation` output groups across a rule and its
attached aspects, aspects and rules can simultaneously use validation
actions.
@fmeum fmeum force-pushed the 19624-merge-validation-outputs branch from e7d115e to caef2f8 Compare September 26, 2023 10:01
@ismell
Copy link

ismell commented Sep 26, 2023

Awesome, thanks for jumping on this so quickly!

@meisterT
Copy link
Member

cc @comius

@ismell
Copy link

ismell commented Oct 4, 2023

cc @lberki

@lberki
Copy link
Contributor

lberki commented Oct 5, 2023

Since both @ahumesky and @comius were tagged into this code review and they didn't respond, I hereby declare it to be Good Enough To Be Merged.

@lberki lberki added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Oct 5, 2023
@copybara-service copybara-service bot closed this in cd72583 Oct 5, 2023
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Oct 5, 2023
@fmeum fmeum deleted the 19624-merge-validation-outputs branch October 5, 2023 21:55
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 5, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Oct 5, 2023
@iancha1992
Copy link
Member

@bazel-io fork 6.4.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Oct 5, 2023
@iancha1992
Copy link
Member

@fmeum @ahumesky @lberki @ismell There were some conflicts when cherry-picking this to release-6.4.0.

  1. src/main/java/com/google/devtools/build/lib/analysis/OutputGroupInfo.java
for (OutputGroupInfo provider : providers) {
  for (String outputGroup : provider.outputGroups.keySet()) {
    if (!seenGroups.add(outputGroup)) {
      throw new DuplicateException(
          "Output group " + outputGroup + " provided twice");
    }

    resultBuilder.put(outputGroup, provider.getOutputGroup(outputGroup));
  }
}

is in release-6.4.0 branch. But expected below before the cherry-pick

for (OutputGroupInfo provider : providers) {
  for (String group : provider) {
    if (outputGroups.put(group, provider.getOutputGroup(group)) != null) {
      throw new DuplicateException("Output group " + group + " provided twice");
    }
  }
}

Also, return new OutputGroupInfo(resultBuilder.buildOrThrow()); is in release-6.4.0 branch currently, which was not expected before the cherry-pick.

cc: @bazelbuild/triage

fmeum added a commit to fmeum/bazel that referenced this pull request Oct 6, 2023
By merging the special `_validation` output groups across a rule and its attached aspects, aspects and rules can simultaneously use validation actions.

Fixes bazelbuild#19624

Closes bazelbuild#19630.

PiperOrigin-RevId: 571084964
Change-Id: I3135ce1f9e61124e96a3b7d8e0ea0c3a3cdf526d

Fixes bazelbuild#19742
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 6, 2023

@iancha1992 I performed a manual cherry-pick in #19745.

iancha1992 pushed a commit that referenced this pull request Oct 6, 2023
By merging the special `_validation` output groups across a rule and its
attached aspects, aspects and rules can simultaneously use validation
actions.

Fixes #19624

Closes #19630.

PiperOrigin-RevId: 571084964
Change-Id: I3135ce1f9e61124e96a3b7d8e0ea0c3a3cdf526d

Fixes #19742
@ismell
Copy link

ismell commented Oct 9, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aspects: Merge validation output group
6 participants